Use correct `GitHubApiDetails` instance for `getApiClient` in `getCodeQLAssetDownloadURL` by mbg · Pull Request #4231 · github/codeql-action

mbg · 2026-10-13T17:45:49Z

Based on #4230 with some cherry-picked commits from #4198 along with a final, new commit that passes the correct GitHubApiDetails instance for getApiClient in getCodeQLAssetDownloadURL to address the point discussed in #4198 (comment).

Risk assessment

For internal use only. Please select the risk level of this change:

Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.

Which use cases does this change impact?

Workflow types:

Advanced setup - Impacts users who have custom CodeQL workflows.
Managed - Impacts users with dynamic workflows (Default Setup, Code Quality, ...).

Products:

Code Scanning - The changes impact analyses when analysis-kinds: code-scanning.
Code Quality - The changes impact analyses when analysis-kinds: code-quality.
Other first-party - The changes impact other first-party analyses.

Environments:

Dotcom - Impacts CodeQL workflows on github.com and/or GitHub Enterprise Cloud with Data Residency.
GHES - Impacts CodeQL workflows on GitHub Enterprise Server.

How did/will you validate this change?

Unit tests - I am depending on unit test coverage (i.e. tests in .test.ts files).
End-to-end tests - I am depending on PR checks (i.e. tests in pr-checks).

If something goes wrong after this change is released, what are the mitigation and rollback strategies?

Rollback - Change can only be disabled by rolling back the release or releasing a new version with a fix.

How will you know if something goes wrong after this change is released?

Telemetry - I rely on existing telemetry or have made changes to the telemetry.
- Dashboards - I will watch relevant dashboards for issues after the release. Consider whether this requires this change to be released at a particular time rather than as part of a regular release.
- Alerts - New or existing monitors will trip if something goes wrong with this change.

Are there any special considerations for merging or releasing this change?

No special considerations - This change can be merged at any time.

Merge / deployment checklist

Confirm this change is backwards compatible with existing workflows.
Consider adding a changelog entry for this change.
Confirm the readme and docs have been updated if necessary.

Copilot

Warning

Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.

🟡 Changes recommended

The API-routing regression lacks direct coverage, and repository validation accepts malformed values.

2 open findings

What changed in this PR

Refactors CodeQL asset resolution so API requests use the correct GitHub instance and authentication details.

Changes:

Extracts repository and asset-resolution logic into a shared module.
Supports unauthenticated GitHub.com API details when running elsewhere.
Updates tests, helpers, generated output, and dependencies.

File	Description
`src/setup/repository.ts`	Adds shared release-asset resolution.
`src/setup/repository.test.ts`	Tests Action repository detection.
`src/api-client.ts`	Supports explicit and Dotcom API details.
`src/setup-codeql.ts`	Uses shared asset resolution.
`src/setup-codeql.test.ts`	Updates setup tests for API details.
`src/testing-utils.ts`	Adds reusable API-details mocking.
`src/codeql.test.ts`	Adopts the shared test helper.
`src/repository.ts`	Adds typed repository NWO strings.
`src/actions-util.ts`	Accepts read-only environments.
`package.json`	Removes a direct dependency.
`package-lock.json`	Updates dependency metadata.
`lib/entry-points.js`	Generated bundle; content excluded.

Files excluded by content exclusion policy (1)

lib/entry-points.js

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…nto it

henrymercer

This broadly looks good. A couple of suggestions:

henrymercer · 2026-10-15T21:17:34Z

+  // We construct the default GitHub.com source here, because `getDotcomApiDetails`
+  // may retrieve the `token` input if we are currently running on GitHub.com and
+  // we want to avoid doing that statically.
+  const defaultDownloadSource: DownloadSource = [


As far as I can tell, retrieving the token input here is what necessitates the changes to the tests including the mockApiDetails helper which you called out as not ideal.

Since we're only using these apiDetails in isSameDownloadSource, we could consider doing something like { auth: undefined, ...getDotcomInstance() } instead to avoid the need to get the token input. I think the mockApiDetails helper could then be removed.

Even if we want to use the token constructed here in the future, I'd argue it'd be more elegant to first migrate code paths to use ActionState, so we can avoid the need to stub getRequiredInput (resulting in serial tests).

As far as I can tell, retrieving the token input here is what necessitates the changes to the tests including the mockApiDetails helper which you called out as not ideal.

Just for the record, the mockApiDetails function and the comment in it aren't new -- I just moved the definition from codeql.test.ts to testing-utils.ts and updated it to return the stub so that I could reset it in one test where I had to use it in a loop.

Since we're only using these apiDetails in isSameDownloadSource, we could consider doing something like { auth: undefined, ...getDotcomInstance() } instead to avoid the need to get the token input. I think the mockApiDetails helper could then be removed.

I think that this would introduce another issue similar to the one that this PR and the previous one in the stack are trying to get rid of. If we explicitly set auth: undefined here now, it will be difficult to identify this issue in the future if this is ever used for something other than isSameDownloadSource here.

Even if we want to use the token constructed here in the future, I'd argue it'd be more elegant to first migrate code paths to use ActionState, so we can avoid the need to stub getRequiredInput (resulting in serial tests).

I'll have a look at making this change, but it depends a bit on how far through the call stack we need to thread it to get the tests to not need mockApiDetails anymore. I don't want to have to refactor all of setup-codeql for this right now.

henrymercer · 2026-10-15T21:25:21Z

+ * are only used with the GitHub instance that is represented by the {@link GitHubApiCombinedDetails} value.
+ */
+export class InstanceApiClient {
+  private readonly client: ApiClient;


Remove this for now since it's unused?

henrymercer · 2026-10-15T21:41:40Z

+  const getRequiredInput = sinon.stub(actionsUtil, "getRequiredInput");
+  getRequiredInput.withArgs("token").returns(apiDetails.auth);


See my other comment about this, but if we keep this then we should at least have the stub call through for other arguments.

…ce-for-getApiClient

github-actions Bot added the size/L May be hard to review label Oct 13, 2026

mbg added this pull request to stack #4232 October 13, 2026 17:46

mbg marked this pull request as ready for review October 14, 2026 09:33

mbg requested a review from a team as a code owner October 14, 2026 09:33

Copilot AI balanced review requested due to automatic review settings October 14, 2026 09:33

Copilot started reviewing on behalf of mbg October 14, 2026 09:34

View session

Copilot AI reviewed Oct 14, 2026

View reviewed changes

Comment thread src/repository.ts

Comment thread src/setup/repository.ts

mbg force-pushed the mbg/use-github-instance-for-getApiClient branch 2 times, most recently from 569b0a1 to 79f834f Compare October 14, 2026 12:47

mbg force-pushed the mbg/use-github-instance-for-getApiClient branch from 79f834f to bd015c2 Compare October 14, 2026 12:54

github-actions Bot added size/XL May be very hard to review and removed size/L May be hard to review labels Oct 14, 2026

mbg added 4 commits October 14, 2026 20:31

Add getDotcomApiDetails

f728bb6

Use instance in DownloadSource and pass it on to getApiClient

f2547e0

Include apiDetails in CodeQLDownloadSource

b2bf750

Add InstanceApiClient and incorporate getAuthorizationHeaderFor i…

8884b5b

…nto it

mbg force-pushed the mbg/use-github-instance-for-getApiClient branch from bd015c2 to 8884b5b Compare October 14, 2026 19:50

github-actions Bot added size/L May be hard to review and removed size/XL May be very hard to review labels Oct 14, 2026

henrymercer reviewed Oct 15, 2026

View reviewed changes

Base automatically changed from mbg/auth/allow-unauthed to main October 16, 2026 11:18

mbg added 3 commits October 16, 2026 12:30

Merge remote-tracking branch 'origin/main' into mbg/use-github-instan…

1772d44

…ce-for-getApiClient

Address feedback from previous PR

1c350b1

Explain when to use getAuthorizationHeaderFor

d9e3972

Navigation Menu

Repository navigation

Use correct `GitHubApiDetails` instance for `getApiClient` in `getCodeQLAssetDownloadURL` - #4231

Use correct `GitHubApiDetails` instance for `getApiClient` in `getCodeQLAssetDownloadURL`#4231
mbg wants to merge 7 commits into
mainfrom
mbg/use-github-instance-for-getApiClient

mbg commented Oct 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

henrymercer left a comment

Uh oh!

henrymercer Oct 15, 2026

Uh oh!

mbg Oct 16, 2026

Uh oh!

henrymercer Oct 15, 2026

Uh oh!

Uh oh!

henrymercer Oct 15, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		const getRequiredInput = sinon.stub(actionsUtil, "getRequiredInput");
		getRequiredInput.withArgs("token").returns(apiDetails.auth);

Conversation

mbg commented Oct 13, 2026

Risk assessment

Which use cases does this change impact?

How did/will you validate this change?

If something goes wrong after this change is released, what are the mitigation and rollback strategies?

How will you know if something goes wrong after this change is released?

Are there any special considerations for merging or releasing this change?

Merge / deployment checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

henrymercer left a comment

Choose a reason for hiding this comment

Uh oh!

henrymercer Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

mbg Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

henrymercer Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

henrymercer Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants